Skip to content

Add primerprospector_analyzeprimers#11615

Merged
vagkaratzas merged 13 commits into
masterfrom
add-primerprospector_analyzeprimers
May 21, 2026
Merged

Add primerprospector_analyzeprimers#11615
vagkaratzas merged 13 commits into
masterfrom
add-primerprospector_analyzeprimers

Conversation

@vagkaratzas
Copy link
Copy Markdown
Contributor

@vagkaratzas vagkaratzas commented May 12, 2026 •
edited
Loading

Workaround shim in main:

Primer Prospector 1.0.1 (this is an old codebase) is a Python 2 package and the BioContainer entrypoint uses Python 2.7.
The bug is specifically Python 2 behavior: range() requires integer arguments, and the package passes a
numpy.float64 during plotting.

PR checklist

  • This comment contains a description of changes (with reason).
  • If you've fixed a bug or added code that should be tested, add tests!
  • If you've added a new tool - have you followed the module conventions in the contribution docs
  • If necessary, include test data in your PR.
  • Remove all TODO statements.
  • Broadcast software version numbers to topic: versions - See version_topics
  • Follow the naming conventions.
  • Follow the parameters requirements.
  • Follow the input/output options guidelines.
  • Add a resource label
  • Use BioConda and BioContainers if possible to fulfil software requirements.
  • Ensure that the test works with either Docker / Singularity. Conda CI tests can be quite flaky:
    • For modules:
      • nf-core modules test <MODULE> --profile docker
      • nf-core modules test <MODULE> --profile singularity
      • nf-core modules test <MODULE> --profile conda
    • For subworkflows:
      • nf-core subworkflows test <SUBWORKFLOW> --profile docker
      • nf-core subworkflows test <SUBWORKFLOW> --profile singularity
      • nf-core subworkflows test <SUBWORKFLOW> --profile conda

timrozday-mgnify
timrozday-mgnify requested changes May 14, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@timrozday-mgnify timrozday-mgnify left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @vagkaratzas ! This looks useful.

I have one major issue with this module relating to incorrect handling of output hit files when there are multiple primers in the input primer file (a common case). The tests should be amended with this case.

I think it would be useful to support gzipped fasta inputs, but I don't know what nf-core good practice says about that.

Comment thread modules/nf-core/primerprospector/analyzeprimers/main.nf Outdated
-f "${fasta_arg}" \\
${primer_arg}

mv *_hits.txt "${prefix}_hits.txt"
Copy link
Copy Markdown
Contributor

@timrozday-mgnify timrozday-mgnify May 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For multiple primers in the input primer file this will cause an error or overwrite some results. Rather all the files should be emitted in the hits output channel. If prefix renaming is desired then it needs to be performed on each of the produced hits files.

Copy link
Copy Markdown
Contributor Author

@vagkaratzas vagkaratzas May 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, updated!

@vagkaratzas
Copy link
Copy Markdown
Contributor Author

vagkaratzas commented May 21, 2026

I think it would be useful to support gzipped fasta inputs, but I don't know what nf-core good practice says about that.

Since the module allows a list of fasta files (and not just a single fasta one), it is a bit more complicated to do that in the code (but definitely not impossilbe). But for now I guess running GUNZIP as an intermediate step can solve that case. The module is already a bit more complicated than usually (due to being old and possibly unreachable authors to update), so I'd leave the decompressing out of the current scope.

timrozday-mgnify
timrozday-mgnify approved these changes May 21, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@timrozday-mgnify timrozday-mgnify left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thank you @vagkaratzas!

Just two edits to the error messages since prefix is no longer used to rename output files.

I've approved so you can crack on with merging when you choose.

Comment thread modules/nf-core/primerprospector/analyzeprimers/main.nf Outdated
Comment thread modules/nf-core/primerprospector/analyzeprimers/main.nf Outdated
@vagkaratzas vagkaratzas added this pull request to the merge queue May 21, 2026
Merged via the queue into master with commit aab1801 May 21, 2026
26 checks passed
@vagkaratzas vagkaratzas deleted the add-primerprospector_analyzeprimers branch May 21, 2026 12:00
manascripts pushed a commit to manascripts/modules that referenced this pull request May 21, 2026
@vagkaratzas @timrozday-mgnify @manascripts
Add primerprospector_analyzeprimers (nf-core#11615)
043778e 
* bots initial attempt

* removed unnecessary temp output dir, and renamed output files, added comment for bug workaround

* flagged the bug in the comment

* execfile on PATH for shim, instead of local user/ path

* stricter version eval to pass GitHub runners warnings

* updated and tested for multiple primers

* Update modules/nf-core/primerprospector/analyzeprimers/main.nf

Co-authored-by: Tim Rozday <timrozday@ebi.ac.uk>

* Update modules/nf-core/primerprospector/analyzeprimers/main.nf

Co-authored-by: Tim Rozday <timrozday@ebi.ac.uk>

* prefix only used in stub

---------

Co-authored-by: Tim Rozday <timrozday@ebi.ac.uk>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@timrozday-mgnify timrozday-mgnify timrozday-mgnify approved these changes

Assignees

No one assigned

Labels

size/m

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@vagkaratzas @timrozday-mgnify